Skip to content

Reduce arena allocations when materializing RocksDB range read results#13273

Open
pchintar wants to merge 2 commits into
apple:mainfrom
pchintar:perf/kvstore-packed-kv-copy
Open

Reduce arena allocations when materializing RocksDB range read results#13273
pchintar wants to merge 2 commits into
apple:mainfrom
pchintar:perf/kvstore-packed-kv-copy

Conversation

@pchintar
Copy link
Copy Markdown

@pchintar pchintar commented May 24, 2026 •
edited
Loading

Closes issue #13272

Summary

Reduce arena allocations when materializing RocksDB range read results.

Currently, every KeyValueRef deep-copy performs two independent arena allocations:

  • one for the key payload,
  • one for the value payload.

This PR instead packs both payloads into a single contiguous arena allocation while preserving existing ownership and StringRef semantics.

Changes

Current implementation:

KeyValueRef(Arena& a, const KeyValueRef& copyFrom)
  : key(a, copyFrom.key), value(a, copyFrom.value) {}

This performs two separate StringRef(Arena&, ...) allocations per returned KV pair.

In contrast, my PR introduces a packed-copy path:

KeyValueRef(Arena& a, const KeyRef& key, const ValueRef& value)

which:

  • allocates a single contiguous arena buffer for both payloads,
  • copies key/value bytes into that shared allocation,
  • constructs KeyRef / ValueRef views over the packed storage.

The RocksDB range-read paths now use:

result.emplace_back_deep(result.arena(), key, value);

instead of constructing a temporary KeyValueRef followed by generic deep-copy construction.

Overall this reduces arena allocation count from:

2 allocations per KV pair
    ->
1 allocation per KV pair

during range-result materialization.

No public APIs, wire formats, or ownership semantics are changed.

Testing

ninja -j2 fdbserver_kvstorelinktest

./bin/linktest/fdbserver_kvstorelinktest

Note: A full benchmark comparison and large-scale simulation workload validation couldn't be performed locally due to my hardware limitations

saintstack
saintstack reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@saintstack saintstack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I think needs formatting?

Comment thread fdbclient/include/fdbclient/FDBTypes.h
: KeyValueRef(a, copyFrom.key, copyFrom.value) {}

bool operator==(const KeyValueRef& r) const { return key == r.key && value == r.value; }
bool operator!=(const KeyValueRef& r) const { return key != r.key || value != r.value; }
Copy link
Copy Markdown
Contributor

@saintstack saintstack May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

@saintstack saintstack closed this May 29, 2026
@saintstack saintstack reopened this May 29, 2026
@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: deb2168
  • Duration 0:03:46
  • Result: ❌ FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: deb2168
  • Duration 0:04:33
  • Result: ❌ FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: deb2168
  • Duration 0:04:37
  • Result: ❌ FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: deb2168
  • Duration 0:03:50
  • Result: ❌ FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: deb2168
  • Duration 0:04:04
  • Result: ❌ FAILED
  • Error: Error while executing command: if [[ $(git diff --shortstat 2> /dev/null | tail -n1) == "" ]]; then echo "CODE FORMAT CLEAN"; else echo "CODE FORMAT NOT CLEAN"; echo; echo "THE FOLLOWING FILES NEED TO BE FORMATTED"; echo; git ls-files -m; echo; if [[ $FDB_VERSION =~ 7\.\3. ]]; then echo skip; else exit 1; fi; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: deb2168
  • Duration 0:35:56
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@pchintar pchintar force-pushed the perf/kvstore-packed-kv-copy branch from deb2168 to 3d0286d Compare May 29, 2026 23:00
@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 29, 2026

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: deb2168
  • Duration 1:00:38
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@pchintar
Copy link
Copy Markdown
Author

pchintar commented May 29, 2026

Looking good. I think needs formatting?

Hi @saintstack I just fixed all the formatting issues by using clang-format & so could you pls re-run the checks once more? thnx

@saintstack saintstack closed this May 30, 2026
@saintstack saintstack reopened this May 30, 2026
@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: 3d0286d
  • Duration 0:22:14
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 3d0286d
  • Duration 0:33:47
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 3d0286d
  • Duration 0:46:08
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: 3d0286d
  • Duration 0:46:09
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: 3d0286d
  • Duration 0:49:32
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: 3d0286d
  • Duration 0:54:33
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Copy Markdown
Contributor

foundationdb-ci commented May 30, 2026

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: 3d0286d
  • Duration 1:01:20
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saintstack saintstack saintstack left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pchintar @foundationdb-ci @saintstack